Open
Conversation
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
31fe0f4 to
af8e147
Compare
📚 Documentation Check Results📦
|
🔒 Cargo Deny Results📦
|
gyuheon0h
changed the title
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1558 +/- ##
==========================================
+ Coverage 70.85% 71.20% +0.35%
==========================================
Files 424 423 -1
Lines 61963 62002 +39
==========================================
+ Hits 43903 44150 +247
+ Misses 18060 17852 -208
🚀 New features to boost your workflow:
|
BenchmarksComparisonBenchmark execution time: 2026-02-18 23:03:32 Comparing candidate commit 71aba36 in PR branch Found 7 performance improvements and 9 performance regressions! Performance is the same for 41 metrics, 2 unstable metrics. scenario:benching deserializing traces from msgpack to their internal representation
scenario:credit_card/is_card_number/37828224631000521389798
scenario:credit_card/is_card_number/x371413321323331
scenario:credit_card/is_card_number_no_luhn/ 378282246310005
scenario:credit_card/is_card_number_no_luhn/378282246310005
scenario:credit_card/is_card_number_no_luhn/37828224631000521389798
scenario:credit_card/is_card_number_no_luhn/x371413321323331
scenario:normalization/normalize_service/normalize_service/[empty string]
scenario:sql/obfuscate_sql_string
CandidateCandidate benchmark detailsGroup 1
Group 2
Group 3
Group 4
Group 5
Group 6
Group 7
Group 8
Group 9
Group 10
Group 11
Group 12
Group 13
Group 14
Group 15
Group 16
Group 17
Group 18
Group 19
BaselineOmitted due to size. |
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
af8e147 to
8354f61
Compare
Artifact Size Benchmark Reportaarch64-alpine-linux-musl
aarch64-unknown-linux-gnu
libdatadog-x64-windows
libdatadog-x86-windows
x86_64-alpine-linux-musl
x86_64-unknown-linux-gnu
|
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
8354f61 to
6389231
Compare
ivoanjo
reviewed
Feb 12, 2026
Member
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
Nice, this was way easier than I was expecting! I've left you a question in PM, otherwise happy to approve :)
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
6389231 to
3b3f4e0
Compare
danielsn
changed the title
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
3b3f4e0 to
af2d1c7
Compare
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
af2d1c7 to
0dca02a
Compare
Clippy Allow Annotation ReportComparing clippy allow annotations between branches:
Summary by Rule
Annotation Counts by File
Annotation Stats by Crate
About This ReportThis report tracks Clippy allow annotations for specific rules, showing how they've changed in this PR. Decreasing the number of these annotations generally improves code quality. |
ivoanjo
reviewed
Feb 18, 2026
Member
ivoanjo
left a comment
ivoanjo
left a comment
There was a problem hiding this comment.
I really like this! Left a few notes :)
Comment on lines
-149
to
+158
| # | ||
| # `rustls-platform-verifier` uses the Windows certificate APIs (Cert*), which live in Crypt32. | ||
| "ws2_32.lib", "advapi32.lib", "userenv.lib", "ntdll.lib", "bcrypt.lib", "ole32.lib", "crypt32.lib" | ||
| "advapi32.lib", | ||
| "bcrypt.lib", | ||
| # `rustls-platform-verifier` uses the Windows certificate APIs (Cert*), which live in Crypt32 | ||
| "crypt32.lib", | ||
| # `hickory-resolver` uses GetAdaptersAddresses, which lives in iphlpapi. | ||
| "iphlpapi.lib", | ||
| "ntdll.lib", | ||
| "ole32.lib", | ||
| "userenv.lib", | ||
| "ws2_32.lib" |
Member
There was a problem hiding this comment.
(Note to other reviewers -- same list of libraries reordered, with only iphlpapi.lib being new)
| PowrProf | ||
| Version) | ||
| Version | ||
| iphlpapi) |
Member
There was a problem hiding this comment.
This part I'm not confident at all in -- maybe worth confirming with @gleocadie that this new dll dependency is fine?
libdd-common/src/lib.rs
Outdated
Comment on lines
348
to
353
Member
There was a problem hiding this comment.
Regardless of the default (I'll comment on that separately), DD_USE_HICKORY_DNS seems to be a bit... too low level?
Maybe flip it around and use DD_FORCE_SYSTEM_DNS/DD_USE_LEGACY_DNS? Or, if not flipping it, DD_USE_BUILTIN_DNS?
libdd-common/src/lib.rs
Outdated
Comment on lines
346
to
347
Member
There was a problem hiding this comment.
Unless I'm mistaken, only profiling is using the to_reqwest_client_builder. Being that the case, since it's already quite limited subset, I'd go with perhaps using hickory_dns by default, and then dropping a note to every library using libdatadog to perhaps include the env var as a release note in case anyone sees any resolver-related issues?
Also, and I think this would make testing a lot easier -- maybe we could take advantage of the above in an additional way: make the use_hickory a regular argument of this method, and have the env var parsing be part of the caller for now.
This enables easier testing of common AND enables us to evolve a bit how this toggling happens without ever needing to touch this code (which seems nice?)
Comment on lines
86
to
135
Member
There was a problem hiding this comment.
Hmmm... this is quite brittle -- for instance reading the docs:
It sounds like hickory may even be used by default even if the feature isn't enabled...? So there's a non-zero chance this test is not only brittle, but possibly misleading?
Looking at the docs, it looks like the reqwest::Client::builder() has also a dns_resolver parameter where a resolver can be passed in.
What if we allowed the resolver to be injected? E.g. basically reproduce what the match config.hickory_dns is doing in https://docs.rs/reqwest/latest/src/reqwest/async_impl/client.rs.html#421 ?
That way our tests could become:
fn test_without_hickory() {
let resolver = GaiResolver::new();
let builder = to_reqwest_client_builder(resolver);
let response = ...
// maybe somehow assert resolver got called?
}
fn test_with_hickory() {
let resolver = HickoryDnsResolver::default();
let builder = to_reqwest_client_builder(resolver);
let response = ...
// maybe somehow assert resolver got called? perhaps even by setting some config where only hickory can resolve?
}
Comment on lines
149
to
174
Member
There was a problem hiding this comment.
This test seems to be somewhat... weird/unfair. That is why do we drop the client before we do the check for hickory but not the system dns? Doesn't that mean that if hickory did actually use an extra thread but cleaned it up on drop, we wouldn't see it?
E.g. I think the test should maybe be exactly the same between both variants, the only difference should be the actual toggling on and off, and us observing the different number of threads, not the order of operations.
Comment on lines
176
to
182
Member
There was a problem hiding this comment.
Is this test... going to be flaky? E.g. I can imagine we're relying on things such as "how long does the background extra thread spawned for dns stay alive" -- or would it stay alive forever? Might be worth doing a quick check that this test is not relying too much on a race.
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
468316b to
ff80914
Compare
Contributor
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🔗 Commit SHA: 71aba36 | Docs | Datadog PR Page | Was this helpful? Give us feedback! |
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
ff80914 to
c1504f3
Compare
danielsn
changed the title
danielsn
force-pushed
the
dsn/hickory-dns
branch
from
c1504f3 to
71aba36
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Motivation
https://github.com/DataDog/dd-source/pull/355564
The non-use of the blocking pool is particularly nice, since it lets us avoid the uncontrolled thread that can break things on forks.
Additional Notes
Anything else we should know when reviewing?
How to test the change?
Describe here in detail how the change can be validated.